Skip to content

Conversation

@srivastava-diya
Copy link
Contributor

@srivastava-diya srivastava-diya commented Dec 1, 2025

Add Conflicting Type Detection to type Error Handler

This update implements support for detecting conflicting type constraints entirely within the type error handler, as recommended. The previously skipped “ allOf conflicting type” test passes with this change in type error handler.

This fixes : #99

To keep the existing type logic isolated and safe, the changes made are as follows:

  • Starting with a set of all allowed JSON types:

    let allowedTypes = new Set(["null", "boolean", "number", "string", "array", "object", "integer"]);

  • now we intersect it with the type set from each type keyword found at the same instance location. If the resulting set becomes empty, that means the schema’s type constraints are mutually exclusive, so I return a single getConflictingTypeMessage(). If the intersection is not empty, then it isn’t a conflict and the normal type-error handling continues as usual.

  • This keeps allOf functioning purely as a simple applicator while placing all conflict detection logic inside the type handler, as intended.

I’ve implemented this and run the full test suite — all tests pass.

Copy link
Collaborator

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple notes and I pushed some additional tests to expose some gaps we need to fill. There were two main issues. One is dealing with "integer" because it's not a true JSON type and has some overlap with "number". The other issue is about removing duplicate type messages. There should only ever be one per schema.

}

if (allowedTypes.size === 0) {
if (failedTypeLocations.length > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's impossible for failedTypeLocations to be empty if allowedTypes is empty, right? If allowedTypes is empty, there must be at least two occurrences of type that don't overlap. That means one must fail.

Copy link
Contributor Author

@srivastava-diya srivastava-diya Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely right here. One must fail. I will refine that .

Regarding integer vs number, I wasn’t fully clear on how to treat them either. I saw the patternProperties test using type:"integer", so I included "integer" in the initial allowedTypes set, . I initially did not include integer in the set which caused this particulr test to fail so i included it,but I now understand the problem: "integer" isn’t a standalone JSON type, and it has a subtype relationship with "number" .I also saw your note about needing to eliminate duplicate type messages so I'll try to incorporate that .

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you forget to remove this if, or did you come up with a reason why it needs to be there?

const errors = [];

if (normalizedErrors["https://json-schema.org/keyword/type"]) {
let allowedTypes = new Set(["null", "boolean", "number", "string", "array", "object", "integer"]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's declare this set of all possible types as a constant at the top. It doesn't need to be recreated every time the function runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jdesrosiers
I did not declare allowedTypes as a constant at the top because it is getting updated ,let me take an example.

  • For Example
allOf: [
  { "type": ["string", "number"] },
  { "type": ["string"] },
  { "type": ["string", "boolean"] }
]
  • Instance

instance = "helloo"

and we have declared and initialized allowedTypes with let allowedTypes = new Set(["null", "boolean", "number", "string", "array", "object", "integer"]); and in the mean while all the typeKeywords are also getting processed one by one , So suppose the 1st typeKeyword the got processed was temp_set = {string,number} (i'm putting them in a set so that i can take intersection). Now we take intersection of this temp_set with allowedTypes.

temp_Set={string,number}
allowedTypes = new Set(["null", "boolean", "number", "string", "array", "object", "integer"]);

//updated allowedTypes
allowedTypes = (temp_Set) intersection (allowedTypes)  //we get string and number so it becomes allowedTypes={string,number}

Now next typeKeyword gets processed i.e string so,

temp_Set={string}
allowedTypes={string,number}

allowedTypes = (temp_Set) intersection (allowedTypes) // becomes allowedTypes={string}

Now at last typeKeyword that gets processed will be string & boolean

temp_Set={string, boolean}
allowedTypes={string}

allowedTypes = (temp_Set) intersection (allowedTypes) // becomes allowedTypes={string}

and hence our instance which was a string passes. and allowedType cannot be taken as constant.

  • Now had our allowedTypes set finally became empty like in this case:
allOf: [
  { "type": ["string"] },
  { "type": ["number"] },
]

That means we have a type Conflict.

  • But if its final size is greater than 1 then no Type conflict and normal type error handling will do the work.

This was my thought process but it seems like I need to do some more work to get the logic right . Thanks a lot for pinpointing the areas i need to consider.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. I was thinking something like this,

const allTypes = new Set(["null", "boolean", "number", "string", "array", "object", "integer"]);

const type = async (normalizedErrors, instance, localization) => {
  if (normalizedErrors["https://json-schema.org/keyword/type"]) {
    let allowedTypes = allTypes;

    for (const schemaLocation in normalizedErrors["https://json-schema.org/keyword/type"]) {
      allowedTypes = allowedTypes.intersection(keywordTypes);
    }

allowedTypes needs to be reassigned because it's accumulating the result of multiple intersections, but we don't need to recreate the initial value every time. The set intersection operation is immutable, so we can create that set once and reuse it for each initialization of allowedTypes.

Signed-off-by: Diya <diyasrivastava2023@gmail.com>
Comment on lines 31 to 32
const keywordTypes = new Set(types);
allowedTypes = intersectTypeSets(allowedTypes, keywordTypes);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need to write custom set intersection logic. Conceptually, if a schema allows numbers, it also allows integers. If you code for that relationship, the rest should all just work.

Suggested change
const keywordTypes = new Set(types);
allowedTypes = intersectTypeSets(allowedTypes, keywordTypes);
const keywordTypes = new Set(types);
if (keywordTypes.has("number")) {
keywordTypes.add("integer");
}
allowedTypes = allowedTypes.intersection(keywordTypes);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right that would be much easier than any custom intersection logic. I've implemented this.

Comment on lines 44 to 46
if (allowedTypes.has("number")) {
allowedTypes.delete("integer");
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a great place to put this code. It would be more cohesive to put it after the for loop that builds allowedTypes. That way all the code that builds allowedTypes is in one place. Then all the code that reads allowedTypes comes after.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure,moved that block right after the for loop that builds allowedTypes ends.

const errors = [];

if (normalizedErrors["https://json-schema.org/keyword/type"]) {
let allowedTypes = new Set(ALL_TYPES);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no reason to create a new set every time you initialize allowedTypes. ALL_TYPES can be a set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed that, ALL_TYPES is now a set instead of an array

}

if (allowedTypes.size === 0) {
if (failedTypeLocations.length > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you forget to remove this if, or did you come up with a reason why it needs to be there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

allOf conflicting type

2 participants